Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameter and BBCode simple values with syntax tokens #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thunderer
Copy link
Owner

Issue #65.

@MrPetovan
Copy link

With
[url=http://example.com]link[/url]
it correctly gives

array (
  0 => 
  Thunder\Shortcode\Shortcode\ParsedShortcode::__set_state(array(
     'text' => '[url=http://example.com]link[/url]',
     'offset' => 0,
     'name' => 'url',
     'parameters' => 
    array (
    ),
     'content' => 'link',
     'bbCode' => 'http://example.com',
  )),
)

@@ -258,6 +258,28 @@ private function lookahead($type)
return $this->position < $this->tokensCount && (empty($type) || $this->tokens[$this->position][0] === $type);
}

private function lookaheadN(array $types)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't lookaheadN() a more general way for lookahead()? If yes, I would probably rewrite lookahead like this:

private function lookahead($type)
{
    return $this->lookaheadN([$type]);
}

@MrPetovan
Copy link

I found a counter-example:
[url= http://example.com/]link[/url]
gives

array (
  0 => 
  Thunder\Shortcode\Shortcode\ParsedShortcode::__set_state(array(
     'text' => '[url= http://example.com/]',
     'offset' => 0,
     'name' => 'url',
     'parameters' => 
    array (
    ),
     'content' => NULL,
     'bbCode' => 'http://example.com',
  )),
)

As you can see, the content is null because the ending / of the parameter URL is used to self-close the shortcode. This is super tricky because you would need to look for a closing tag first before deciding it's a closing tag.

Copy link

@MrPetovan MrPetovan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those changes don't parse [url= http://example.com/]link[/url] correctly.

@MrPetovan
Copy link

Alternatively, is it possible to have a parser that ignore self-closing tags like this [ ... /]? We don't use this syntax at all over at Friendica.

@thunderer
Copy link
Owner Author

@MrPetovan I'm sorry but I won't be able to continue work on this PR for now. You can try disabling self-closing tags by removing the part handling them in RegularParser::shortcode() and changing the RegularParser::value() from this PR to include closing mark as a valid character. I'd be very grateful if you could work it out into PR we could discuss in the future.

@MrPetovan
Copy link

No problem, thanks for letting me know. Do you still plan on maintaining the project?

@thunderer
Copy link
Owner Author

@MrPetovan Yes, this project is very stable and I'm committed to maintaining it. It's just a temporary state where I won't have any time for side projects right now.

I'm still thinking about your issue, though. It is clearly different from my closing-mark-always-closes-shortcode approach. Such cases are the very reason I introduced parameter delimiters arg="value" and escaping arg="value\"" to resolve all possible ambiguities in the syntax. Is it viable for your use case to use delimiters and let ambiguities be resolved the way they are in this PR? I don't know how much "UX" your users expect, but maybe documentation update will be sufficient? I will try to research how to make RegularParser's behavior configurable to the extent you require when I find time.

@MrPetovan
Copy link

Unfortunately I can’t expect Friendica users to start using delimiters to get the same end result as before we started to use your library. Backward compatibility with the current parser is a blocker, no matter how janky it currently is.

@Fedcomp
Copy link

Fedcomp commented May 20, 2018

I think arguments with spaces is separate issue, and this one is critical for my project. Surprised it wasn't yet merged in. #65 is pretty much fixed by this PR.

@thunderer thunderer force-pushed the simple-parameter-extended-value branch from 590cbfb to f1660ae Compare December 9, 2018 23:21
@thunderer
Copy link
Owner Author

Hi @MrPetovan and @Fedcomp, I'd like to revive this PR as I think I've found a way to make both sides happy. I implemented a WIP commit here. The idea behind this commit is that there will be two value matching modes, standard which is the current behavior and aggressive where value basically stops at whitespace or closing tag ]. If there is a self-closing marker / + closing tag ] combo, it still consumes the closing marker / and makes the shortcode still "open". This allows constructions like [url=http://google.com/]text[/url] to be parsed correctly according to your requirements. Value matching mode is currently controlled by public $valueMode property and VALUE_REGULAR or VALUE_AGGRESSIVE constants - please look at an example in tests.

I would be very grateful if you could test these changes and provide some more test cases so that I can polish the code and prepare final PR. By the way, please look at #72 which brings ~20x performance gain to RegularParser with ~10x memory reduction. The next stable release should be released soon.

@MrPetovan
Copy link

Hi @thunderer and thanks for keeping at it! I'll try to see what I can test over the next week.

@thunderer
Copy link
Owner Author

@MrPetovan I just tagged v0.7.0 with all the improvements and fixes from #72. Did you have the chance to test this PR with your data? Let's give Friendica users the full BBCode power! 😄

@MrPetovan
Copy link

Not yet, I didn't take the time for it. We are in a release candidate period which means that we're focusing on fixing bugs rather than introducing new features. But as soon as we release the next version, I'll be able to focus my time on this. It's been a long-standing issue and I'm eager to finally put it to rest.

@thunderer thunderer force-pushed the simple-parameter-extended-value branch from 9351dfa to 63cafc5 Compare October 27, 2020 22:38
@MrPetovan
Copy link

I won't have the time to test this but it shouldn't stop you from merging. Thank you for your work!

@thunderer
Copy link
Owner Author

@MrPetovan what is the current status of shortcodes in Friendica? Will you be able to use the library now?

@MrPetovan
Copy link

Thanks for asking, not in the immediate future, but it still is planned, and this delay has nothing to do with the quality of this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants